fix: anchor workflow commands for create-architecture#1066
fix: anchor workflow commands for create-architecture#1066dickymoore wants to merge 6 commits intobmad-code-org:mainfrom
Conversation
WalkthroughThese changes modify the workflow command generation system to ensure all workflow paths are anchored to the project root. A new helper function standardizes path formatting, the template is updated to use a new placeholder, and a test validates the path anchoring behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
dickymoore
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tools/cli/installers/lib/ide/templates/workflow-command-template.md (1)
12-12: Note: Additional step unrelated to path anchoring.This new step is a useful best practice but appears unrelated to the PR's stated objective of fixing path anchoring. Consider whether this should be in a separate PR or documenting it in the PR description.
tools/cli/installers/lib/ide/shared/workflow-command-generator.js (1)
115-118: Consider handling leading slashes in path normalization.The
withProjectRoothelper could produce double slashes if a path starts with/:
- Input:
/foo/bar→ Output:{project-root}//foo/barWhile current usage doesn't trigger this (paths are relative like
.bmad/...orbmad/...), it would be more robust to normalize:const withProjectRoot = (p) => { if (!p) return p; + const cleanPath = p.startsWith('/') ? p.substring(1) : p; - return p.startsWith('{project-root}/') ? p : `{project-root}/${p}`; + return cleanPath.startsWith('{project-root}/') ? cleanPath : `{project-root}/${cleanPath}`; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/test-installation-components.js(1 hunks)tools/cli/installers/lib/ide/shared/workflow-command-generator.js(3 hunks)tools/cli/installers/lib/ide/templates/workflow-command-template.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Focus on inconsistencies, contradictions, edge cases and serious issues.
Avoid commenting on minor issues such as linting, formatting and style issues.
When providing code suggestions, use GitHub's suggestion format:<code changes>
Files:
tools/cli/installers/lib/ide/templates/workflow-command-template.mdtools/cli/installers/lib/ide/shared/workflow-command-generator.jstest/test-installation-components.js
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: CLI tooling code. Check for: missing error handling on fs operations,
path.join vs string concatenation, proper cleanup in error paths.
Flag any process.exit() without error message.
Files:
tools/cli/installers/lib/ide/shared/workflow-command-generator.jstest/test-installation-components.js
🧬 Code graph analysis (1)
tools/cli/installers/lib/ide/shared/workflow-command-generator.js (1)
test/test-installation-components.js (1)
workflow(169-174)
🔇 Additional comments (5)
tools/cli/installers/lib/ide/templates/workflow-command-template.md (1)
8-8: LGTM! Template change aligns with path anchoring objective.The replacement of
@{bmad_folder}/core/tasks/workflow.xmlwith@{{core_workflow_path}}ensures the core workflow path is anchored to the project root, which is properly populated by the generator at line 137-145 in workflow-command-generator.js.test/test-installation-components.js (1)
160-187: LGTM! Test correctly validates path anchoring.The test properly verifies that:
- Workflow paths are anchored to
{project-root}/- Old relative
@.bmadpatterns that caused IDE resolution issues are eliminatedThe test path flows correctly through the generator's
withProjectRoothelper (line 134 in generator) to produce the expected anchored format.tools/cli/installers/lib/ide/shared/workflow-command-generator.js (3)
120-138: LGTM! Path extraction and anchoring logic is sound.The flow correctly handles both source paths (from
/src/modules/or/src/core/) and installed paths (like.bmad/...), then consistently anchors them to{project-root}/. ThecoreWorkflowPathconstruction ensures the core workflow is always properly anchored.
145-145: LGTM! Template replacement for core workflow path.The
{{core_workflow_path}}replacement correctly injects the anchored core workflow path computed on line 137, aligning with the template changes.
233-235: LGTM! Handles already-installed paths correctly.The new else-if branch ensures that installed workflow paths (e.g.,
.bmad/bmm/...) that don't match the source path patterns (/src/modules/or/src/core/) are properly anchored to{project-root}/. This complements the anchoring logic ingenerateCommandContentfor display paths in launcher files.
|
@dickymoore good idea here, but going to close as will be removing all need for workflow.xml once the final workflows are converted. |
2 participants
{project-root}so IDEs don’t resolve core/workflow paths relative to module folders; add regression test for thecreate-architectureworkflow command.*workflow-init/*create-architecturecommands could fail with “workflow.xml does not exist” because generated command paths were relative (e.g.,.bmad/bmm/core/tasks/workflow.xml). Anchoring ensures commands point to.bmad/core/tasks/workflow.xmland the correct workflow file.{project-root}and anchor display paths for installed workflows.create-architecturecommand.npm testI double-checked upstream main (55cb468) and the anchoring fix isn’t present:
This looks like a bug, not intentional behavior:
Given the symptom and template intent, anchoring to {project-root} matches the design, while the current relative output is erroneous.
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes
Updates
✏️ Tip: You can customize this high-level summary in your review settings.